-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: nav stack back native #2656
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @chrismclarke. Your changes seem good, I like the use of signals and nice to have the back button handling logic separated out within the nav-stack feature folder.
Testing on device I noticed that by default capacitor/angular appears to disable default back-button navigation behaviour when a custom interceptor added, meaning that repeated back button presses appear to close stack, followed by nav-back and finally minimise. I think this is the behaviour we want, although just to note that this hasn't been explicitly coded in this PR (could be done if we notice things not functioning as expected on particular devices or in future capacitor versions).
Noted.
I've added one commit, 1b40fc3, to make the closeAllNavStacks()
method close all open nav stacks simultaneously (closing them sequentially was a compromise to support the logic in #2594).
In terms of other changes from #2655, there is still the issue, as you note, of users pressing the browser back button and the underlying template navigating without closing any open nav-stacks. Whilst it's true that we can expect users to close nav-stacks via in-app means, I think that it would be good to handle this behaviour if it's not too difficult to do so.
I've opened #2659 which proposes incorporating the back button listener logic from #2655 into the new nav-stack-back service. Let me know if you think that's desirable or if the logic is too messy to include.
PR Checklist
Description
Intercepts hardware back-button presses (android only), to close the top nav stack (if open)
Author Notes
The work initially included handling back-button press in browser, but quickly became over-complicated and messy. As such pressing the browser back button will still not close nav stacks and just navigate the underlying template. It's expected that users on browsers will close nav stacks using the close button or clicking outside of the stack.
On android, however, the hardware back button functions independently the the rest of the navigation system, and so it is easier to intercept hardware back-button presses and add custom functionality. In this case pressing the hardware back button will close the top nav-stack if open.
Review Notes
The feature is only enabled on android, so requires local build/run on an android emulator to test
Dev Notes
The code started from #2594 to keep the improved handling of tracking nav stack arrays, although the custom navigation code was removed and tracking array converted to signals.
A new service has been added to better-encapsulate the custom back-button interception. This could later be extended if adding further functionality on web.
Testing on device I noticed that by default capacitor/angular appears to disable default back-button navigation behaviour when a custom interceptor added, meaning that repeated back button presses appear to close stack, followed by nav-back and finally minimise. I think this is the behaviour we want, although just to note that this hasn't been explicitly coded in this PR (could be done if we notice things not functioning as expected on particular devices or in future capacitor versions).
I'm not sure if this would supercede/close #2655 or not - both PRs were worked on at a similar time in parallel so unclear whether there is still functionality in 2655 that would be nice to have or not
Git Issues
Closes #2594
Closes #2569
Screenshots/Videos
2024-12-27.10-50-10.webm